-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🚧 stop_and_save_print_to_ram
should not overwrite valid partial backup
#4410
Conversation
All values in bytes. Δ Delta to base
|
stop_and_save_print_to_ram
should not overwrite valid partial backupstop_and_save_print_to_ram
should not overwrite valid partial backup
9d37ea4
to
ea3985f
Compare
Rebased to sync with MK3 branch (105 commits) |
ea3985f
to
a53fd06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as the removed saved...
values are refreshed using refresh_print_state_in_ram
function.
memcpy(saved_pos, current_position, sizeof(saved_pos)); | ||
if (!isPartialBackupAvailable) { | ||
refresh_print_state_in_ram(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A problem I see here with this approach, is that now set_temp_error saves current_position before planner_abort_hard() is called, so current_position is not correct during a thermal error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gudnimg Is this solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I havent had a good opportunity to look into this. I was too sick to do any productive things previous weekend
I will hopefully look at this again soon (and other PRs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gudnimg is this solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the PR needs to be reworked to address this. I think we should postpone this after 3.14
Thermal errors need to take a backup of the target temperatures. When the print is then later paused, stop_and_save_print_to_ram should not overwrite the partial backup if it is still valid. Change in memory: Flash: -150 bytes SRAM: 0 bytes
a53fd06
to
673f0f8
Compare
Synced PR with latest MK3 branch (~126 commits) |
stop_and_save_print_to_ram
should not overwrite valid partial backupstop_and_save_print_to_ram
should not overwrite valid partial backup
@gudnimg Is it still in progress or can that be reviewed? |
@3d-gussner This pull request needs more work, the comment from @wavexx is still valid and I haven't looked more into this. Closing PR for now since I'm unlikely to finish it soon. |
Thermal errors need to take a backup of the target temperatures before disabling the heaters. When the print is then later paused,
stop_and_save_print_to_ram
should use the partial backup if it is still valid (isPartialBackupAvailable=true
). Afterstop_and_save_print_to_ram
is done running and we have a 'full' backup,isPartialBackupAvailable
is cleared in favor ofsaved_printing
Change in memory:
Flash: -156 bytes
SRAM: 0 bytes